fix(auth): Single-User Multiple-Email Issue using CM-Keycloak #7728
Conversation
bivanalhar
changed the title
bivanalhar
force-pushed
the
bivan/keycloak-multi-email
branch
2 times, most recently
from
203f5ea to
57fc741
Compare
| it 'sets another email as primary' do | ||
| subject | ||
| expect(non_primary_email.reload).to be_primary |
There was a problem hiding this comment.
Looking at the old spec here, there's no issue deleting primary email since it will set another confirmed email as primary right?
In this case, it is unnecessary to introduce a user constraint to prevent primary email deletion.
There was a problem hiding this comment.
Either this, or user.emails.set_primary.no_confirmed_emails is now redundant
There was a problem hiding this comment.
true, but as discussed before, we want to make the behavior in FE and BE in this regards to be consistent (FE does not allow deletion of primary email, and hence its related controller in BE should also have such requirements, and the testing needs to be done also to reflect on this change)
As for no_confirmed_emails, this is still relevant since actually even though controller prohibits deletion of primary email, it can still be done through other means (one example being to delete course_user, in which user_emails are one of its children and hence cascade delete takes place)
There was a problem hiding this comment.
Ok to update test case to change to disallow deletion of primary. But other test isn't necessary right, because user_emails tied to user, not course_user?
cysjonathan
changed the title
bivanalhar
force-pushed
the
bivan/keycloak-multi-email
branch
2 times, most recently
from
e98a64e to
9a31752
Compare
bivanalhar
force-pushed
the
bivan/keycloak-multi-email
branch
from
9a31752 to
e62be73
Compare
Problem Statement
Natively, Keycloak handles the case of different emails as different entities entirely, even though they might actually be used by the same user (Coursemology supports multiple emails to be associated with one user). This leads to the problem of when a user logs in using 2 different emails, then in other services (such as Cikgo or Koditsu) those 2 emails will be regarded as belonging to 2 different users, which is wrong. This PR resolves this particular issue
Approach
Previously we setup the user ID inside Keycloak by using an ID registered under
user_emailstable. Because we want to regard all emails from a single user as the same entity under Keycloak, we instead use an ID fromuserstable. In this way, no matter which email is being used by the user, they will all be directed towards one single entity by Keycloak and hence will be regarded correctly as being the same user across platform as well.This approach has a drawback, though, in which should a user has an unconfirmed email being registered, then logging in using any confirmed email, then later on logging in using this unconfirmed one within different session will gives user a successful login (since Keycloak already cached the user's data inside their own DB)
Therefore, to mitigate this issue, we also add the filtering of all unconfirmed email out of the search results everytime Keycloak makes a query to Coursemology DB. This will ensure that user won't be able to login using unconfirmed email and hence only be able to login using confirmed one
Trade-Off
Even though the proposed approach resolves the single-user multiple-email issue, as well as keeping the logging in through unconfirmed email impossible, there is a trade-off in the behaviour of logging in through unconfirmed email.
Previously, if we do that, we will be redirected towards the
Unconfirmed Emailpage, in which there will be a hyperlink to trigger Coursemology to resend the confirmation email to user. Now, doing so will only render the email invalid and hence only the warning of "Invalid username or password" will be generated, and no redirection will happenOther Issues being Resolved
Other than single-user multiple-email issue, I also noticed that there is no validation that occurs while trying to delete the email through controller. By rights, we should not be able to delete a primary email (this has been handled well in our Frontend), but right now we can merely call the API for destroy primary email directly and it will be successful. Therefore, I added the gate-keeping of such in this PR as well